-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
python: use Python Environment Tools for interpreter discovery #5897
Conversation
E2E Tests 🚀 |
@@ -689,7 +689,7 @@ | |||
"type": "string" | |||
}, | |||
"python.locator": { | |||
"default": "js", | |||
"default": "native", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the rust locator is considered experimental and is opt-in only for upstream. I've swapped it for testing purposes, but we'll want to decide what we want as the default. I've played around with it locally, and it seems to be finding everything and is MUCH FASTER, eg, 1-3 seconds to find my 80+ envs. We should do a bit more robust testing before deciding though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had the same experience. We could leave the default as js
when we merge to main
so that it's optional for the Feb release, and make it the default on main
for devs to test during Feb in preparation for the Mar release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Once this is merged and we have a build, I can make a discussion post asking for people to opt-in for more feedback 🕺
python-version: 3.8 | ||
cache: 'pip' | ||
cache-dependency-path: | | ||
extensions/positron-python/requirements.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to add a working directory for files in .github/actions/
? It doesn't look like the way we set it in the Python CI works in this directory.
positron/.github/workflows/positron-python-ci.yml
Lines 18 to 20 in 14bff09
defaults: | |
run: | |
working-directory: 'extensions/positron-python' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @isabelizimm! I checked out the branch, ran npm install
, then tested interpreter discovery in a dev build. It seems to work well and is much faster.
Looks good to merge once CI is green and we've decided on the default behavior.
@@ -689,7 +689,7 @@ | |||
"type": "string" | |||
}, | |||
"python.locator": { | |||
"default": "js", | |||
"default": "native", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had the same experience. We could leave the default as js
when we merge to main
so that it's optional for the Feb release, and make it the default on main
for devs to test during Feb in preparation for the Mar release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, LGTM! I only have 5 interpreters and it's probably about the same speed. So that's another good data point!
await app.workbench.interpreter.selectInterpreter('Python', desiredPython); | ||
await app.workbench.interpreter.verifyInterpreterIsSelected(desiredPython); | ||
await app.workbench.interpreter.verifyInterpreterIsRunning(desiredPython); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
simple!
After fighting with extension smoke tests, I swapped the default back to We've tried this out in builds, and it does not seem to have the same errors. I feel pretty confident this is a temp dir weirdness since the e2e tests are passing. It looks like upstream has moved to an entirely different testing strategy for the smoke tests, which we might want to look into. |
Release Notes
New Features
Bug Fixes
QA Notes
Should be able to locate interpreters/start up Python runtimes with setting
"python.locator": "native"
TODO